Skip to content

Issue #50: Register the Header in recipe registry #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 30, 2025
Merged

Issue #50: Register the Header in recipe registry #59

merged 1 commit into from
Jul 30, 2025

Conversation

@Anmol202005
Copy link
Collaborator

@Anmol202005 Anmol202005 commented Jul 21, 2025

Fixes: #50
Implemented Header recipe in recipe registry

@Anmol202005
Copy link
Collaborator Author

Anmol202005 commented Jul 21, 2025

changes:

  • check names format was not consistent through out the project. So, i have changed it to UpperCase throughout the project.
  • registered header in recipe registry
  • updated CheckstyleRecipeRegistryTest to extend AbstractXmlTestSupport to create a config rather than creating an external config file.

@rdiachenko rdiachenko requested a review from timurt July 21, 2025 18:10
timurt
timurt reviewed Jul 22, 2025
View reviewed changes
Copy link
Collaborator

@timurt timurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments from my side

@Anmol202005
Copy link
Collaborator Author

Anmol202005 commented Jul 23, 2025

@timurt ping

rdiachenko
rdiachenko reviewed Jul 24, 2025
View reviewed changes
timurt
timurt approved these changes Jul 25, 2025
View reviewed changes
Copy link
Collaborator

@timurt timurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from my side

Thank you!

rdiachenko
rdiachenko requested changes Jul 25, 2025
View reviewed changes
@rdiachenko rdiachenko assigned rdiachenko and unassigned timurt Jul 25, 2025
@Anmol202005 Anmol202005 force-pushed the registryimpl branch 2 times, most recently from 10788b2 to d564d17 Compare July 28, 2025 11:14
@Anmol202005
Copy link
Collaborator Author

Anmol202005 commented Jul 28, 2025

@rdiachenko done.

rdiachenko
rdiachenko reviewed Jul 28, 2025
View reviewed changes
@Anmol202005 Anmol202005 force-pushed the registryimpl branch 3 times, most recently from 79dfd2c to 0ac8465 Compare July 29, 2025 21:17
rdiachenko
rdiachenko approved these changes Jul 30, 2025
View reviewed changes
Copy link
Member

@rdiachenko rdiachenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not ideal:

  • createRecipe could return optional instead of null
  • if (check.isPresent()) could be replaced with more readable stream api like check.ifPresent

But we already spent too much time on this PR, and it blocks further progress. I'm ok to merge it

@rdiachenko rdiachenko merged commit f223f4a into checkstyle:main Jul 30, 2025
3 checks passed
@rdiachenko rdiachenko mentioned this pull request Aug 3, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rdiachenko rdiachenko rdiachenko approved these changes

@timurt timurt timurt approved these changes

Assignees

@rdiachenko rdiachenko

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement Header in CheckstyleAutoFix

3 participants

@Anmol202005 @rdiachenko @timurt